Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ADP-3226] Use more IsRecentEra #4422

Merged
merged 4 commits into from
Feb 8, 2024

Conversation

HeinrichApfelmus
Copy link
Contributor

@HeinrichApfelmus HeinrichApfelmus commented Jan 31, 2024

This pull request changes some code to align with the decision "Singleton type RecentEra :: AP".

In some places, I have

  • Removed an explicit RecentEra era argument in favor of the type class / implicit argument Write.IsRecentEra era — unless this would have resulted in an ambiguous type, such as for mkTransaction.
  • Removed @era applications that are not of the form recentEra @era.
    (I have left applications of the form shelleyBasedEra @era and cardanoEra @era for convenience, as they are similar in spirit.)

Comments

  • This pull request is an alternative to [Experimental] Simplify the use of recent eras #4409 .

  • I have kept or even added an explicit RecentEra era argument to the functions buildTransaction, constructTransaction, and constructUnbalancedSharedTransaction, because their usage site (Server module) is prone to ambiguity. The era type is introduced through an existential like this

            (Write.PParamsInAnyRecentEra era pp, _)
                <- liftIO $ W.readNodeTipStateForTxWrite netLayer

    In order to get the era type into scope, we would need to write

            (Write.PParamsInAnyRecentEra (_era :: RecentEra era) pp, _)
                <- liftIO $ W.readNodeTipStateForTxWrite netLayer

    In both cases, the value or type are needed to specify in which era the SealedTx should be created.

Issue Number

ADP-3226

@HeinrichApfelmus HeinrichApfelmus self-assigned this Jan 31, 2024
@HeinrichApfelmus HeinrichApfelmus force-pushed the HeinrichApfelmus/ADP-3226/IsRecentEra branch 5 times, most recently from 91851b2 to 24d10e0 Compare February 1, 2024 17:34
@HeinrichApfelmus HeinrichApfelmus marked this pull request as ready for review February 1, 2024 17:43
@HeinrichApfelmus HeinrichApfelmus requested review from Anviking and jonathanknowles and removed request for Anviking February 1, 2024 17:43
Copy link
Member

@jonathanknowles jonathanknowles left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me!

(Modulo one small comment about a $ that can be removed.)

scriptData
aux
val
removeDummyInput RecentEraConway = \case
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice 👍🏻

lib/wallet/src/Cardano/Wallet/Shelley/Transaction.hs Outdated Show resolved Hide resolved
@@ -1198,19 +1199,21 @@ prop_sealedTxRecentEraRoundtrip
sealedTxB = sealedTxFromBytes' currentEra txBytes

makeShelleyTx
:: IsRecentEra era
:: Write.IsRecentEra era
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why use IsRecentEra qualified but not RecentEra?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Historical reasons only. RecentEra was unqualified before I touched the file; I at least wanted to qualify all IsRecentEra that I touched.

@@ -1146,13 +1146,14 @@ binaryCalculationsSpec' era = describe ("calculateBinary - "+||era||+"") $ do
calculateBinary net utxo outs chgs pairs =
hex (Cardano.serialiseToCBOR ledgerTx)
where
ledgerTx :: Cardano.Tx (CardanoApiEra era)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cannot satisfy: cardano-ledger-binary-1.2.1.0:Cardano.Ledger.Binary.Version.MinVersion <= Ledger.ProtVerHigh era0 without type annotation 🤔 maybe we should have kept the era arg here then?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was the error I saw when testing dropping era from constructTransaction also 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the presence of GADTs, it is good practice — and sometimes necessary — to annotate polymorphic types in where or let bindings.

More information can be found in the documentation for the MonoLocalBinds extensions, which is implied by the GADTs extension.

In other words: Using GADTs has a cost. MonoLocalBinds is that cost. We pay it by sometimes adding type signatures.

Co-authored-by: Johannes Lund <anviking@me.com>
Co-authored-by: Jonathan Knowles <mail@jonathanknowles.net>
@HeinrichApfelmus HeinrichApfelmus force-pushed the HeinrichApfelmus/ADP-3226/IsRecentEra branch from 8801dd0 to 4f2c631 Compare February 8, 2024 11:54
@HeinrichApfelmus HeinrichApfelmus added this pull request to the merge queue Feb 8, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Feb 8, 2024
@HeinrichApfelmus HeinrichApfelmus added this pull request to the merge queue Feb 8, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Feb 8, 2024
@HeinrichApfelmus HeinrichApfelmus added this pull request to the merge queue Feb 8, 2024
Merged via the queue into master with commit ebf229f Feb 8, 2024
3 checks passed
@HeinrichApfelmus HeinrichApfelmus deleted the HeinrichApfelmus/ADP-3226/IsRecentEra branch February 8, 2024 14:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants